-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a "separator" attribute to post-terms block #32812
Conversation
Size Change: +141 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
Changed the default from a pipe ( |
This works great in my testing. The only thing I'm not sure about is whether this needs to live in the Advanced section — since this block doesn't have very many controls to begin with, I think it might as well be highlighted elsewhere in the sidebar. Adding |
This reminds me a lot of #23293 The designs in that issue all seemed to default to showing this option in the sidebar and also included a spacing and size option — all under it's own accordion section: -- I think it could make sense to expose something like this in a block's toolbar, and the post-terms block is a good example. There are minimal controls in the toolbar already, and controlling the separators feels important enough to the block's needs to show in the toolbar. With that in mind, I wonder if there's a way to have one UI that will work both in the sidebar and in the toolbar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work Ari!
Something like this will be needed. It's not clear to me what the suggested design is though? @shaunandrews is this the design suggested to be explored but in the toolbar? If yes, even though it might be something |
f606ec6
to
727ae05
Compare
"Spacing" makes sense, but I would skip "size" to keep things more straightforward. I don't think this belongs in the toolbar. |
463c2c2
to
51d2535
Compare
51d2535
to
1483bf8
Compare
}, | ||
"separator": { | ||
"type": "string", | ||
"default": ", " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should default be empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a comma makes more sense — it tends to be the most commonly used separator in themes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this PR needs the spacing
control as well, no? With the spacing control we should remove the extra space from the default value ,
as this is not visible in the input and might lead to some confusion there.
There's nothing stopping us from adding spacing controls in future iterations, but I'm unsure whether that would be a requirement or not for a 1st iteration. The "separator characters(s)" control would always be needed regardless of whether we have spacing or not, so IMO it would be safer to go with something small and if we see that spacing is required and requested, then add it. Personal opinion: I was thinking that the spacing control would probably add too much complexity for something that may not necessarily need that much... The space character may not be directly visible when editing the block, but when entering the control and actually editing the value, it becomes self-evident and is extremely simple to use. Granted, it's not perfect, but maybe it's the lesser of 2 evils? As a user I'd rather hit the spacebar once or twice than have to edit a bunch of separate controls 😅 |
Yeah — assuming we stick with the comma as a default separator, I think a spacing control is unnecessary for this iteration. |
Okay then, let's land this first iteration as is, but first we should add a style to the separator to preserve
Since JSX removes whitespace. You can try this by just adding more than one space in separator. |
Great catch! Done in e5c8fb8 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ari! I'm pre approving but you still need to just import the new style here: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/style.scss
That's all that is left.
--edit
I pushed the import 😄
68cc112
to
d5bb6b2
Compare
* Add a "separator" attribute to post-terms * Change default from a pipe to a comma * fix defaults * Add separator class * update fixtures * change selector from sep to separator * Add styles * indentation fix * import style Co-authored-by: ntsekouras <ntsekouras@outlook.com>
Description
Fixes #31710
Adds a "Separator" attribute to post-terms. The control is exposed in the "Advanced" section of the post-categories and post-tags blocks.
How has this been tested?
,
and confirmed that it works both in the editor and the frontendChecklist:
*.native.js
files for terms that need renaming or removal).